Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Split performStream into functions handling stream of futures #68

Merged
merged 2 commits into from
Sep 18, 2019
Merged

Split performStream into functions handling stream of futures #68

merged 2 commits into from
Sep 18, 2019

Conversation

paldepind
Copy link
Member

This PR changes

function performStream<A>(s: Stream<IO<A>>): Now<Stream<A>>;
function performStreamLatest<A>(s: Stream<IO<A>>): Now<Stream<A>>;
function performStreamOrdered<A>(s: Stream<IO<A>>): Now<Stream<A>>;

into

function performStream<A>(s: Stream<IO<A>>): Now<Stream<Future<A>>>;

function flatFuture<A>(stream: Stream<Future<A>>): Now<Stream<A>>;
function flatFutureLatest<A>(stream: Stream<Future<A>>): Now<Stream<A>>;
function flatFutureOrdered<A>;

In other words, there is now a single function for running a stream and it results in a stream of futures. To flatten/unnest this stream of futures one then has to use one of the three new functions for doing so. But, these new functions can be used in any circumstance where one has a stream of futures.

We can discuss the names of the last three function. The logic behind the current names is that flat is a function that turns M<M<A>> into M<A> while satisfying things like flat(map(v => M.of(v)) === v and the new functions are similar in that flatFuture turns a Stream<Future<A>> into a Stream<A> while satisfying things like flatFuture(map(v => Future.of(v), stream)) === stream. Hence the idea is that highlighting the similarity by using a familiar word "flat" will make it easier to understand and remember what this new function does.

Related to: funkia/purescript-hareactive#3

@paldepind paldepind requested review from limemloh and Jomik September 7, 2019 06:55
@codecov

This comment has been minimized.

Sign in to view
@codecov
Copy link

codecov bot commented Sep 7, 2019 •
edited
Loading

Codecov Report

Merging #68 into master will increase coverage by 0.45%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage   95.79%   96.25%   +0.45%     
==========================================
  Files          13       13              
  Lines        1476     1494      +18     
  Branches      130      135       +5     
==========================================
+ Hits         1414     1438      +24     
+ Misses         62       56       -6
Impacted Files Coverage Δ
src/now.ts 92.3% <100%> (-0.09%) ⬇️
src/stream.ts 95.68% <100%> (+0.83%) ⬆️
src/behavior.ts 98.28% <100%> (+0.06%) ⬆️
src/testing.ts 96.66% <100%> (+1.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa02e03...1e3b3ff. Read the comment docs.

@paldepind
Copy link
Member Author

Here's a diagram, inspired by the one @stevekrouse did in #57 that tries to show the difference between the flatFuture{Latest,Ordered} functions.

flatFuture-1

paldepind
paldepind commented Sep 7, 2019
View reviewed changes
.travis.yml
@@ -1,7 +1,6 @@
language: node_js
node_js:
- "node"
- "8"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the test implementations use Array#flatMap which this old NodeJS version does not support.

Jomik
Jomik approved these changes Sep 12, 2019
View reviewed changes
Copy link
Member

@Jomik Jomik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing jumps in my eye. LGTM

@limemloh
Copy link
Member

Really nice work 😄

My only comment is that I don't find the name flatFuture suitable and I would even dare to call it a bit misleading. Although it shares some properties with flat/flatten.
With a name like flatFuture I would expect it to have the type:

Future<Future<A>> -> Future<A>

where the actual type is:

Stream<Future<A>> -> Now<Stream<A>>

the only difference in the naming is the addition of ...Future which I do not find helpful.

the idea is that highlighting the similarity by using a familiar word "flat" will make it easier to understand and remember what this new function does.

It is nice grouping functions which are sharing properties together, but I find it more important to use a name, which describes the behavior of a function rather then the properties of a function and I also believe it to be easier to remember.

Instead I suggest something like arrange or organize, which would be better at describing the behavior of the function, and works well with the variations for instance arrangeOrdered or organizedOrdered.

@paldepind
Copy link
Member Author

I think flatFuture is a great name because what flatFuture does closely resembles what flat does on a stream. flat on a stream turns a Stream<Stream<A>> into a Stream<A> and flatFuture turns a Stream<Future<A>> into a Stream<A>.

Not only are the types similar. The behavior is also very similar. flat(streamOfStreams) flattens the nested structure by taking all the values from the inner streams and returning a new stream of them. Simiarly flatFuture(streamOfFutures) flattens the nested structure by taking all the values from the inner futures and returning a new stream of them.

On top of the useful analogy with flat I think the name flatFuture captures quite well what the function does. It flattens futures down into a stream.

On the other hand I don't think arrange communicates what the function does. In a call such as arrange(streamOfFutures) I don't really see how the function "arranges" anything. It just immedtiately delivers the values from the futures as they show up.

@paldepind
Copy link
Member Author

I've renamed flatFuture to flatFutures. This better indicates that many futures are flattened in the stream.

@limemloh limemloh merged commit 88e22e7 into master Sep 18, 2019
@snyk-bot snyk-bot mentioned this pull request Mar 30, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jomik Jomik Jomik approved these changes

@limemloh limemloh Awaiting requested review from limemloh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paldepind @limemloh @Jomik